refactor(core/entityTag): Refactor Entity Tags to React #3717

christopherthielen · 2017-05-19T18:54:27Z

Introduce imperative React Modal API
Introduce Formsy validation for React

I guess I should fix the tests too

christopherthielen · 2017-05-19T18:55:16Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+}
+
+@autoBindMethods
+class EntityTagMessage extends React.Component<IEntityTagMessageProps, {}> {


I split these components out, but left them in the same file. Good? Bad? Separate files? The option component is teeny and I didn't think deserved a file.

Small enough, only used here....

christopherthielen · 2017-05-19T18:56:08Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+  ReactInjector, TaskMonitor, TaskMonitorBuilder
+} from 'core';
+
+import { BasicFieldLayout, TextArea, ReactModal } from 'core/presentation';


these imports were imported from 'core' before, and ended up being used while they were still undefined because of timing and dependency loops

christopherthielen · 2017-05-19T18:56:25Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+  entityRef: IEntityRef;
+  isNew: boolean;
+  show?: boolean;
+  onHide?(event: any): void;


Do yall like or hate this syntax?

works for me! doesn't look like there are arguments though?

The caller can access the trigger event from the click event handler or whatever. IDK if it's necessary.

I used onHide.apply to propagate any arguments

christopherthielen · 2017-05-19T18:58:04Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+    const { isNew, tag, ownerOptions } = this.props;
+    const message = this.state.message || '';
+
+    const closeButton = (


This may deserve its own component in its own file.

React modal has a closeButton={true} support but it uses stock bootstrap markup, which doesn't look the same. In the future, a PR to react-bootstrap allows a component to be passed like closeButton={DeckCloseButton}

christopherthielen · 2017-05-19T18:58:27Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+      </div>
+    );
+
+    const { TaskMonitorWrapper } = NgReact;


Dereference NgReact components inside render to avoid trouble

christopherthielen · 2017-05-19T19:07:54Z

app/scripts/modules/core/src/presentation/formsy/components/Input.tsx

+export interface IState extends IFormComponentState { }
+
+/** A simple Formsy form component for validated <input> tags (text or checkbox) */
+export class Input extends FormComponent<string, IProps, IState> {


I'm not actually using this component, but this shows a simpler approach which doesn't accept a layout strategy.

christopherthielen · 2017-05-19T19:08:35Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+ */
+@autoBindMethods()
+export class TextArea extends FormComponent<string, IProps, IState> {
+  public static contextTypes = {


This grants the TextArea component access to the formsy context object.

Type your context, then contextTypes has type React.ValidationMap<ITextAreaContext>

your context

christopherthielen · 2017-05-19T19:09:16Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+    const isInvalid = this.showError() || this.showRequired();
+    const isDirty = !this.isPristine();
+    const inputClass = `form-control ${className} ${isInvalid ? 'ng-invalid' : ''} ${isDirty ? 'ng-dirty' : ''}`;
+    const _input = <textarea className={inputClass} rows={rows} name={name} onChange={this.changeValue} value={this.getValue()} />;


This builds the actual <textarea> element and wires the data and events

christopherthielen · 2017-05-19T19:09:44Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+    const _error = errorMessage && isDirty && <span className="error-message">{errorMessage}</span>;
+
+    const FormFieldLayout = layout;
+    return (


This delegates to the layout, passing it the 4 elements: label, input, help, error

christopherthielen · 2017-05-19T19:10:16Z

app/scripts/modules/core/src/presentation/formsy/formsy-react.d.ts

+// Type wrapper for Formsy-React
+// Project: https://github.com/christianalfoni/formsy-react
+
+declare module 'formsy-react' {


there are no @types/formsy-react, so this is a very incomplete typings file for it

anotherchrisberry

Some generally minor stuff but LGTM overall - very nice work.

anotherchrisberry · 2017-05-19T19:11:25Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+  private $uibModalInstanceEmulation: IModalServiceInstance & { deferred?: IDeferred<any> };
+
+  /** Shows the Entity Tag Editor modal */
+  public static show(props: IEntityTagEditorProps): Promise<any> {


Promise<void>?

In the generic sense, it (Modal.show) actually does resolve to the "onHide action". This could be used to return a promise for a radio choice in a modal, for example.

In this case (EntityTagEditor) it doesn't resolve to anything, so changing to void is proper

anotherchrisberry · 2017-05-19T19:12:35Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+      isValid: false,
+      isSubmitting: false,
+      owner: owner,
+      entityType: this.props.entityType,


move to destructuring above

anotherchrisberry · 2017-05-19T19:14:50Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+      application: application,
+      title: `${isNew ? 'Create' : 'Update'} ${this.props.tag.value.type} for ${entityRef.entityId}`,
+      modalInstance: this.$uibModalInstanceEmulation,
+      onTaskComplete: () => this.props.onUpdate(),


destructure?

anotherchrisberry · 2017-05-19T19:17:22Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+            <button type="submit" className="btn btn-primary" disabled={!this.state.isValid || this.state.isSubmitting}>
+              <span className="glyphicon glyphicon-ok-circle" />
+              <span>{this.props.isNew ? ' Create' : ' Update'} {this.props.tag.value.type}</span>
+            </button>


Might be worth replacing with SubmitButton - might not be, what do I know

Are you suggesting a submit button that's reused elsewhere (code in its own file), or simply extracting it to a const?

there already exists a <SubmitButton> React component in there somewhere...you just have to find it.

anotherchrisberry · 2017-05-19T19:18:41Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+
+@autoBindMethods
+class EntityTagMessage extends React.Component<IEntityTagMessageProps, {}> {
+  private handleTextareaChanged(event: React.FormEvent<any>): void {


any can probably be HTMLTextAreaElement

anotherchrisberry · 2017-05-19T19:26:00Z

app/scripts/modules/core/src/entityTag/entityUiTags.component.ts

-  public constructor(private $timeout: ng.ITimeoutService, private $uibModal: IModalService,
-                     private confirmationModalService: any, private entityTagWriter: EntityTagWriter) {
+  public constructor(private $timeout: ng.ITimeoutService,
+                     private confirmationModalService: any,


this can be typed to ConfirmationModalService

also import ITimeoutService from angular

anotherchrisberry · 2017-05-19T19:27:28Z

app/scripts/modules/core/src/help/HelpField.tsx

+  };
+
+  private helpContentsRegistry: HelpContentsRegistry;
+  private helpContents: any;


can this be typed to something?

{ [key: string]: string } probably

or the IHelpContents type from below

anotherchrisberry · 2017-05-19T19:29:00Z

app/scripts/modules/core/src/help/HelpField.tsx

+    }
+  }
+
+  public render(): React.ReactElement<any> {


remove return type

anotherchrisberry · 2017-05-19T19:31:04Z

app/scripts/modules/core/src/presentation/formsy/BasicFieldLayout.tsx

+ * |                 |                                       ||
+ * |                 +---------------------------------------+|
+ * +----------------------------------------------------------+
+ */


nice ascii art!

anotherchrisberry · 2017-05-19T19:35:10Z

app/scripts/modules/core/src/presentation/index.ts

@@ -1,3 +1,5 @@
 export * from './HoverablePopover';
 export * from './Tooltip';
 export * from './LabelComponent';
+export * from './ModalService';
+export * from './formsy/index';


prefer export * from './formsy since there's an index in there

jrsquared

just a few questions and style things, but LGTM. great work!

jrsquared · 2017-05-19T20:50:03Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+  entityRef: IEntityRef;
+  isNew: boolean;
+  show?: boolean;
+  onHide?(event: any): void;


jrsquared · 2017-05-19T20:53:28Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+      </div>
+    );
+
+    const submitButton = (


Might make sense to use SubmitButton and add support for custom button text to support the {tag.value.type} bit.

jrsquared · 2017-05-19T20:53:55Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+    return (
+      <Modal show={this.state.show} onHide={this.onHide} dialogClassName="entity-tag-editor-modal">
+
+        <TaskMonitorWrapper monitor={this.state.taskMonitor} />


jrsquared · 2017-05-19T20:54:59Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+}
+
+@autoBindMethods
+class EntityTagMessage extends React.Component<IEntityTagMessageProps, {}> {


Small enough, only used here....

jrsquared · 2017-05-19T20:55:34Z

app/scripts/modules/core/src/entityTag/EntityTagEditor.tsx

+          <TextArea
+            label="Message"
+            _help={<div>Markdown is okay <HelpField id="markdown.examples"/></div>}
+            layout={BasicFieldLayout}


jrsquared · 2017-05-19T21:03:41Z

app/scripts/modules/core/src/presentation/formsy/components/Input.tsx

+
+import { FormComponent, IFormComponentState, IFormComponentProps } from '../FormComponent';
+
+export interface IProps extends IFormComponentProps {


Where did we land on exported props names? I like the pattern of if its exported to name it...

This is exported because Typescript requires it, but it is not re-exported from the module index:

export { Input } from './Input'; export { TextArea } from './TextArea';

cool?

jrsquared · 2017-05-19T21:05:07Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+
+  public static defaultProps = {
+    name: null as any,
+    onChange: () => null as any,


whats the benefit of null as any over {}?

I've imported noop from core/utils in other projects which I think worked nicely. Then you don't have to keep creating the same () => {} everywhere

jrsquared · 2017-05-19T21:06:18Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+ */
+@autoBindMethods()
+export class TextArea extends FormComponent<string, IProps, IState> {
+  public static contextTypes = {


Type your context, then contextTypes has type React.ValidationMap<ITextAreaContext>

jrsquared · 2017-05-19T21:06:38Z

app/scripts/modules/core/src/presentation/formsy/components/TextArea.tsx

+    formsy: PropTypes.any
+  };
+
+  public static defaultProps = {


default props should have a type of Partial<IProps>

jrsquared · 2017-05-19T21:07:03Z

app/scripts/modules/core/src/presentation/formsy/formFieldLayout.ts

+
+/** An interface which Formsy form field layouts should accept as props */
+export interface IFormFieldLayoutProps extends HTMLAttributes<any> {
+  _label?: React.ReactElement<any>;


so many underscores...

- Introduce imperative React Modal API - Introduce Formsy validation for React

- Introduce imperative React Modal API - Introduce Formsy validation for React - chore(travis): run yarn

christopherthielen requested review from jrsquared and anotherchrisberry May 19, 2017 18:54

christopherthielen commented May 19, 2017

View reviewed changes

anotherchrisberry reviewed May 19, 2017

View reviewed changes

jrsquared reviewed May 19, 2017

View reviewed changes

christopherthielen force-pushed the edit-entity-tags branch from 6e0e1b2 to 3f93109 Compare May 19, 2017 22:45

christopherthielen added 5 commits May 19, 2017 16:04

refactor(core/entityTag): Refactor Entity Tags to React

85fc37f

- Introduce imperative React Modal API - Introduce Formsy validation for React

chore(travis): run yarn

a408272

Addressing PR comments

6c1ae46

Use <SubmitButton> from somewhere else

4b1b067

Addressing more PR comments

cb5f853

christopherthielen force-pushed the edit-entity-tags branch from 3f93109 to cb5f853 Compare May 19, 2017 23:04

christopherthielen merged commit e96db46 into spinnaker:master May 19, 2017

chris-h-phillips pushed a commit to chris-h-phillips/deck that referenced this pull request May 25, 2017

refactor(core/entityTag): Refactor Entity Tags to React (spinnaker#3717)

7b8bf58

- Introduce imperative React Modal API - Introduce Formsy validation for React - chore(travis): run yarn

christopherthielen deleted the edit-entity-tags branch June 2, 2017 15:41


		import { FormComponent, IFormComponentState, IFormComponentProps } from '../FormComponent';

		export interface IProps extends IFormComponentProps {

refactor(core/entityTag): Refactor Entity Tags to React #3717

refactor(core/entityTag): Refactor Entity Tags to React #3717

Conversation

christopherthielen commented May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anotherchrisberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrsquared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherthielen commented May 19, 2017 •

edited

Loading